backport: bitcoin#25383, 25535, 25337#6524
Conversation
|
This pull request has conflicts, please rebase. |
vijaydasmp
force-pushed
the
Jan_2025_04
branch
from
d82b8b5 to
d82edfc
Compare
vijaydasmp
force-pushed
the
Jan_2025_04
branch
3 times, most recently
from
6d99da7 to
9afdab1
Compare
|
This pull request has conflicts, please rebase. |
vijaydasmp
force-pushed
the
Jan_2025_04
branch
from
9afdab1 to
f1c649f
Compare
vijaydasmp
force-pushed
the
Jan_2025_04
branch
from
f1c649f to
3be5fd7
Compare
vijaydasmp
changed the title
vijaydasmp
force-pushed
the
Jan_2025_04
branch
6 times, most recently
from
007a285 to
f246628
Compare
vijaydasmp
changed the title
WalkthroughThe changes update several aspects of wallet functionality and its testing. In wallet interfaces, the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📥 CommitsReviewing files that changed from the base of the PR and between c5a70462a8af12a32593b5e5b1227e793f518391 and a7d4127. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/wallet/wallet.cpp (2)
2903-2910: Add documentation about concurrencyThis method relies on the wallet lock being held but does not explicitly mention it in the function signature or comments. Consider clarifying that the caller must hold
cs_walletto avoid confusion.
2927-2940: Clarify handling of empty “purpose” argumentWhen the “purpose” parameter is empty, the logic adds all non-change labels to the returned set. Consider documenting this fallback scenario or validating that it matches the intended design for filtering address labels by purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 26ea618 and 2573d091234384e17175eca188c441a6d3a85a53.
📒 Files selected for processing (10)
src/interfaces/wallet.h(1 hunks)src/wallet/bdb.cpp(0 hunks)src/wallet/interfaces.cpp(1 hunks)src/wallet/rpcwallet.cpp(6 hunks)src/wallet/wallet.cpp(1 hunks)src/wallet/wallet.h(1 hunks)src/wallet/walletdb.cpp(2 hunks)test/functional/feature_dbcrash.py(2 hunks)test/functional/wallet_basic.py(1 hunks)test/functional/wallet_listreceivedby.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/wallet/bdb.cpp
🧰 Additional context used
🪛 Cppcheck (2.10-2)
src/wallet/interfaces.cpp
[error] 225-225: Syntax Error
(internalAstError)
🔇 Additional comments (20)
src/interfaces/wallet.h (1)
137-137: Excellent improvement for const-correctness.Adding the
constqualifier to thegetAddresses()method properly indicates that it doesn't modify the wallet's state. This follows C++ best practices and enables calling the method on const instances of theWalletinterface.test/functional/wallet_listreceivedby.py (1)
65-68: Good test enhancement for address classification.This addition improves test coverage by validating that no addresses returned by
listreceivedbyaddress()are change addresses. This helps ensure that the address filtering logic properly distinguishes between receiving addresses and change addresses, which is crucial for accurate wallet display and accounting.test/functional/wallet_basic.py (1)
30-30: Improved test configuration with more specific parameter.Replacing
-acceptnonstdtxn=1with-dustrelayfee=0is a good refinement that:
- Uses a more targeted approach to handling small-value outputs
- Maintains other transaction standardness rules while specifically addressing dust handling
- Provides better alignment with how small outputs are handled in production environments
This change makes the tests more precise without broadly bypassing all standardness checks.
Also applies to: 34-34
src/wallet/walletdb.cpp (2)
883-885: Enhanced wallet version handling.The addition of the
has_last_clientvariable properly tracks whether the last client version was successfully read from the database. This improves logging clarity by showing both the wallet version and client version in a single message.
907-907: More robust version update logic.The revised condition
!has_last_client || last_client != CLIENT_VERSIONensures the version is properly updated both when it's missing from the database and when it differs from the current client version. This provides better handling for new wallets and migration scenarios.test/functional/feature_dbcrash.py (3)
65-66: Improved documentation clarity for node configurationThe comment has been updated to better clarify that node3 will handle "dust" outputs, which aligns with the parameter change below.
66-66: Configuration parameter update from-acceptnonstdtxnto-dustrelayfee=0The change replaces the older non-standard transaction acceptance flag with a more specific dust relay fee setting. This provides a more targeted approach for testing with small value outputs by explicitly setting a zero dust threshold rather than broadly accepting non-standard transactions.
220-224: Improved UTXO creation strategy and verificationThe UTXO creation process has been refactored from a single large batch of 5000 UTXOs to 5 smaller batches of 1000 each. This approach:
- Keeps the same total number of UTXOs (5000)
- Provides more granular control over the creation process
- Adds a verification step to ensure the mempool is empty after UTXO generation
This change improves test robustness by breaking the work into smaller chunks and adding explicit verification.
src/wallet/wallet.h (3)
797-802: Added a flexible filtering structure for address book operationsThe new
AddrBookFilterstructure provides a clean way to filter address book entries by label and whether to include change addresses. The default behavior (ignoring change addresses) is sensible as change addresses are typically not relevant for most user-facing operations.
804-813: Added flexible method for retrieving filtered address book dataThe
ListAddrBookAddressesmethod enables more flexible retrieval of address data by accepting an optional filter parameter. This is a good improvement over the removedGetLabelAddressesmethod as it provides more powerful filtering capabilities.
814-819: Added utility methods for address book operationsThe new methods
ListAddrBookLabelsandForEachAddrBookEntryprovide a clean interface for:
- Retrieving all labels for a specific purpose
- Iterating through address book entries with a callback function
These additions follow good programming practices by:
- Using a callback pattern for iteration that avoids exposing internal data structures
- Providing specific functionality for common address book operations
- Making the code more maintainable and easier to understand
src/wallet/interfaces.cpp (2)
208-219: Updated address lookup to use the new FindAddressBookEntry methodThe implementation of
getAddresshas been simplified by usingFindAddressBookEntryinstead of manually checking the address book. This change improves code maintainability by:
- Reducing duplicate code
- Centralizing the logic for finding address book entries
- Using a consistent approach for address book access across the codebase
221-230: Added const qualifier and improved address retrieval implementationTwo key improvements in this change:
- The
getAddressesmethod is now marked asconst, correctly indicating that it doesn't modify the wallet state- The implementation now uses the new
ForEachAddrBookEntrycallback method instead of direct iterationThis implementation:
- Is more concise and easier to understand
- Properly filters out change addresses
- Uses a consistent pattern for address book access
- Correctly constructs the result vector in a single operation
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 225-225: Syntax Error
(internalAstError)
src/wallet/wallet.cpp (1)
2911-2926: Verify behavior for empty labelsThe filter logic correctly handles both ignore-change and optional label matching. However, if the label filter is set but the address has an empty label, the function will skip it. Verify if this is intended or if an empty label should be handled differently (e.g., treat empty label as a valid match).
src/wallet/rpcwallet.cpp (6)
543-543: Data structure change from set to vectorThis change from
std::set<CTxDestination>tostd::vector<CTxDestination>alters the fundamental container type for addresses. While vectors offer better cache locality, be aware that checking for address existence now has O(n) complexity instead of O(log n) that sets provide.
548-548: API migration to more flexible address filteringThe code now uses the new
ListAddrBookAddressesmethod withAddrBookFilterinstead of the olderGetLabelAddresses. This is part of a broader refactoring to provide more versatile address book querying capabilities.
559-559: Switched from set insertion to vector appendChanged from
address_set.insert(dest)toaddress_vector.emplace_back(dest), consistent with the container type change. Note thatemplace_backis generally more efficient thanpush_backfor complex objects as it constructs the element in-place.
579-579: Modified address existence checkThe address lookup has changed from the efficient
address_set.count(address)to the linear searchstd::find(address_vector.begin(), address_vector.end(), address) != address_vector.end(). This maintains the same functionality but may be less efficient for large address collections.
3949-3966: Refactored label-based address retrieval using ForEachAddrBookEntryThe implementation of
getaddressesbylabelhas been updated to use a lambda with the newForEachAddrBookEntrymethod, replacing a direct loop over address book entries. This approach is more maintainable as it centralizes address book iteration logic and simplifies the calling code.The lambda function now directly constructs and adds the value object to the results instead of using the removed
AddressBookDataToJSONfunction.
4013-4013: Updated label retrieval to use new APIThis line now uses
pwallet->ListAddrBookLabels(purpose)rather than directly iterating through the address book entries, which is consistent with the address book API modernization.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/wallet/interfaces.cpp (1)
217-218: Direct access to entry fieldsWhile
GetLabel()is used for the label field, direct access is used for the purpose field. Consider creating a getter method for purpose as well for consistency.src/wallet/rpcwallet.cpp (4)
543-543: Consider potential performance implications of using a vector instead of a set.
If the size ofaddress_vectorgrows large, membership checks withstd::findoperate in O(n) time, which might degrade performance compared to a set-based approach.
579-579: Watch out for repeated linear searches onaddress_vector.
Usingstd::findevery time could introduce inefficiency if the vector grows large.
1000-1001: Lambda captures everything by reference; confirm no unintended captures.
Capturing[&]is convenient but requires careful maintenance if more external variables are used inside the lambda.
1041-1047: Consider extracting this filtered-address logic into its own function.
The nested if-else logic from lines 1041 to 1047 could be refactored for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 2573d091234384e17175eca188c441a6d3a85a53 and 6398dc8c778b49c5bf6673f72b33c03d1d82f66c.
📒 Files selected for processing (8)
src/interfaces/wallet.h(1 hunks)src/wallet/interfaces.cpp(1 hunks)src/wallet/rpcwallet.cpp(9 hunks)src/wallet/wallet.cpp(1 hunks)src/wallet/wallet.h(1 hunks)test/functional/feature_dbcrash.py(2 hunks)test/functional/wallet_basic.py(1 hunks)test/functional/wallet_listreceivedby.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/interfaces/wallet.h
- test/functional/wallet_basic.py
- test/functional/wallet_listreceivedby.py
- test/functional/feature_dbcrash.py
🧰 Additional context used
🪛 Cppcheck (2.10-2)
src/wallet/interfaces.cpp
[error] 225-225: Syntax Error
(internalAstError)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (14)
src/wallet/interfaces.cpp (3)
208-209: Clean implementation of address lookupThe switch to using
FindAddressBookEntrywith explicitallow_change=falseparameter simplifies the code and makes the intent clearer.
211-212: Usage of accessor method for labelGood use of the
GetLabel()accessor method rather than directly accessing the label field.
221-229: Improved implementation using ForEachAddrBookEntryThe refactoring makes good use of the new
ForEachAddrBookEntrymethod to iterate over address book entries. The lambda function properly filters out change addresses and creates the result vector with all necessary information. The method is correctly marked asconstto indicate it doesn't modify the wallet state.The code properly declares exclusive lock requirements in the lambda to ensure proper concurrent access handling.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 225-225: Syntax Error
(internalAstError)
src/wallet/wallet.h (3)
797-802: Well-designed filter structureThe
AddrBookFilterstruct is well-designed with appropriate use ofstd::optionalfor the label and sensible defaults. The comments clearly explain the purpose of each field.
804-807: Flexible address book filtering methodThe
ListAddrBookAddressesmethod provides a flexible way to filter address book entries with the optional filter parameter. It's correctly marked asconstand requires the wallet lock.
809-812: Clear method for retrieving address labelsThe
ListAddrBookLabelsmethod provides a clean way to retrieve unique labels associated with a specific purpose. Using astd::setfor the return type appropriately ensures uniqueness.src/wallet/wallet.cpp (3)
2903-2909: Well-structured iteration over address book entries.This helper method cleanly encapsulates the iteration logic, allowing the caller to easily process each address book entry. The read-only usage (function is const) is safe, and there are no obvious correctness or performance issues.
2911-2923: Parameter-driven address filtering is effective.The use of an optional filter object and a lambda to exclude change addresses or addresses not matching a specified label is straightforward and efficient. The
AssertLockHeld(cs_wallet)precondition ensures thread safety for reading the address book. No issues are evident.
2927-2939: Appropriate label retrieval functionality.This method correctly gathers distinct labels, ignoring change entries if specified, and matches a requested purpose if provided. Lock acquisition (
AssertLockHeld(cs_wallet)) is properly enforced. Implementation appears correct with no security or performance concerns.src/wallet/rpcwallet.cpp (5)
548-548: Verify correctness of the newListAddrBookAddressesusage.
Ensure all call sites that previously expected addresses fromGetLabelAddressescorrectly handle the list returned byListAddrBookAddresses.
559-559: Emplacing single destination into the vector looks correct.
No immediate issues spotted; consistent with the shift to a vector container.
954-954: Usage ofstd::optional<CTxDestination>is clear and self-explanatory.
This approach provides a robust way to handle the presence or absence of a filtered address.
1004-1004: Early return looks correct.
Prevents unnecessary processing when there is no matching tally entry andfIncludeEmptyis false.
1050-1051: Logic for label-based tally is straightforward.
This block nicely handles the aggregation of amounts by label.
src/wallet/wallet.h
Outdated
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential inconsistency in ForEachAddrBookEntry documentation
The documentation comment states that iteration "Stops when the provided 'ListAddrBookFunc' returns false", but the function type is defined with a void return type, which can't return a boolean value to control iteration.
Verify that either:
- The callback function should return a boolean (in which case the function type should be changed), or
- The documentation should be updated to match the actual behavior
🏁 Script executed:
#!/bin/bash
# Check the implementation of ForEachAddrBookEntry to see if it expects a boolean return
grep -A 15 "void CWallet::ForEachAddrBookEntry" src/wallet/wallet.cppLength of output: 840
Update documentation for ForEachAddrBookEntry to correctly reflect its behavior
There is an inconsistency: the documentation claims that iteration stops when the callback returns false, yet the callback type is defined as void and the implementation iterates over all entries without checking any return value. Please update the comment to indicate that the address book is fully iterated without an early termination condition.
- Location:
src/wallet/wallet.h(Lines 814–819) - Action: Revise the documentation comment to remove any reference to stopping on a false return value.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/wallet/wallet.h (1)
814-819:⚠️ Potential issueUpdate documentation for ForEachAddrBookEntry
There is an inconsistency in the documentation comment. It states that iteration "Stops when the provided 'ListAddrBookFunc' returns false", but the function type is defined with a
voidreturn type, which can't return a boolean to control iteration.Please update the documentation to accurately reflect the behavior of the function. Either:
- Change the comment to indicate that all entries are always processed, or
- Modify the
ListAddrBookFunctype to return a boolean if early termination is desired
🧹 Nitpick comments (3)
src/wallet/wallet.cpp (1)
2903-2909: Consider asserting the wallet lock for consistency.
The function iterates overm_address_book, yet it does not explicitly assertcs_walletis held. Other similar methods (ListAddrBookAddresses,ListAddrBookLabels) do so. To maintain consistency and clarity about concurrency requirements, consider adding the lock assertion here as well.Here is a possible change:
void CWallet::ForEachAddrBookEntry(const ListAddrBookFunc& func) const +{ + AssertLockHeld(cs_wallet); for (const auto& item : m_address_book) { ... } }src/wallet/rpcwallet.cpp (2)
543-559: Consider potential performance trade-offs when using a vector instead of a set.These lines replace the previous set-based structure with a vector (
address_vector) for storing addresses. While vectors can be more cache-friendly and typically faster for small collections, lookups viastd::findare O(n), which may be a concern for larger address lists. If lookups are expected to happen frequently on a large collection, consider using a suitable set-like container or caching lookups to maintain efficiency.
1000-1008: Consider renaming the lambda for clarity.The introduced lambda function named
funcis correct, but giving it a more descriptive name (e.g.,processAddressBookEntry) would improve readability.- const auto& func = [&](const CTxDestination& address, const std::string& label, const std::string& purpose, bool is_change) { + const auto& processAddressBookEntry = [&](const CTxDestination& address, const std::string& label, const std::string& purpose, bool is_change) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 6398dc8c778b49c5bf6673f72b33c03d1d82f66c and bfd640ca97faa79abff93dc053a4f5bf3c792594.
📒 Files selected for processing (6)
src/interfaces/wallet.h(1 hunks)src/wallet/interfaces.cpp(1 hunks)src/wallet/rpcwallet.cpp(9 hunks)src/wallet/wallet.cpp(1 hunks)src/wallet/wallet.h(1 hunks)test/functional/wallet_listreceivedby.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/interfaces/wallet.h
- test/functional/wallet_listreceivedby.py
🧰 Additional context used
🪛 Cppcheck (2.10-2)
src/wallet/interfaces.cpp
[error] 225-225: Syntax Error
(internalAstError)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
🔇 Additional comments (14)
src/wallet/interfaces.cpp (3)
208-210: Improved address lookup withFindAddressBookEntryThe change from direct map access to the
FindAddressBookEntrymethod provides better encapsulation and explicit handling of change addresses. This approach is more robust and maintainable.
211-218: Updated accessor pattern for address book entriesThe switch from
it->second.nametoentry->GetLabel()aligns with the new address book structure. The method now properly handles the updated data structure while maintaining the same behavior.
221-228: Enhanced implementation withconstqualifier and callback patternThe addition of the
constqualifier correctly indicates this method doesn't modify wallet state. The implementation is improved by replacing direct iteration with theForEachAddrBookEntrycallback pattern, which provides better encapsulation and cleaner code.Note: The cppcheck error on line 225 appears to be a false positive related to the
EXCLUSIVE_LOCKS_REQUIREDthread safety annotation.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 225-225: Syntax Error
(internalAstError)
src/wallet/wallet.cpp (2)
2911-2925: Implementation looks correct.
This function properly locks the wallet (AssertLockHeld(cs_wallet)) and usesForEachAddrBookEntrywith a concise lambda to filter addresses by change status and optional label. The design is straightforward and appears robust.
2927-2939: Straightforward label filtering.
The method is clear, ignoring change addresses and collecting labels that match the specified purpose. This approach is consistent with the rest of the address-book-related methods.src/wallet/wallet.h (3)
796-802: Clean and well-structured AddrBookFilter structGood addition of a filtering structure with sensible defaults. The optional label filter and the change address exclusion flag provide flexible querying capabilities for address book entries.
804-808: Good API design for ListAddrBookAddressesThis method introduces a more flexible way of retrieving address book entries with filtering capabilities. This is a good enhancement over the removed
GetLabelAddressesmethod, allowing for more powerful queries.
809-813: Nicely implemented ListAddrBookLabels methodThis method is well-designed to retrieve a set of unique labels for a given purpose. Using a
std::setis the appropriate data structure for maintaining uniqueness of labels.src/wallet/rpcwallet.cpp (6)
954-959: LGTM: Address filtering logic is well-structured.The conditional checks (
!by_label && params.size() > 4) and the subsequent decoding ofparams[4]ensure safe usage. The new filtering approach for a singlefiltered_addressis straightforward and correct, and invalid addresses are robustly handled. No further changes required.
974-985: Clear conditional checks for filtered addresses and ownership.This loop properly extracts the destination, checks it against the optional
filtered_address, and verifies wallet ownership via(mine & filter). The flow guards against invalid or irrelevant outputs and should behave correctly in both the filtered and unfiltered scenarios.
1013-1013: Logic for tallying known addresses is clear.The early check on
it != mapTally.end()prevents erroneous lookups, and the subsequentelsebranch handles addresses not already added to the tally. The flow remains consistent, with no obvious logical or correctness issues.Also applies to: 1024-1024
1032-1034: Good addition of transaction IDs to the response.Collecting and pushing back the
txidsinto the JSON array provides clear traceability of transactions associated with each address. Implementation looks correct and should not introduce performance concerns.
1040-1040: No issues with lambda closure.This is simply the end brace for the lambda, and the closure is syntactically correct.
1042-1052: Well-handled branching for optional address filtering.By either processing a single
filtered_addressor iterating over the entire address book, the code comprehensively covers both scenarios. The subsequent logic referencinglabel_tallyandby_labelis straightforward and consistent with the rest of the function.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/wallet/rpcwallet.cpp (4)
543-559: Check for potential duplicates when using a vector instead of a set.Switching from a set to a vector means insertions no longer enforce uniqueness. If duplicates are undesirable here, consider preserving the set-based structure or add extra checks to prevent duplicates.
580-580: Consider performance implications of linear membership checks.Using
std::findon astd::vectoris O(n). If the vector can grow large, you may consider astd::unordered_setorstd::setto reduce lookup costs.
979-979: Suggest clarifying the comparison with optional.Rather than
if (filtered_address && !(filtered_address == address)), explicitly comparefiltered_address.value() != addressfor readability:- if (filtered_address && !(filtered_address == address)) { + if (filtered_address && filtered_address.value() != address) {
985-985: Improve clarity of conditional check.
if (!(mine & filter))could be made clearer by using an equality check:- if (!(mine & filter)) + if ((mine & filter) == 0)This emphasizes that no bits are set in common.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 7beb411a2a206907d2b1fc7dbb70063a24b1c3ef and bb26584fb836ed10239e56644fbbd7ecd428224c.
📒 Files selected for processing (8)
src/interfaces/wallet.h(1 hunks)src/wallet/interfaces.cpp(1 hunks)src/wallet/rpcwallet.cpp(9 hunks)src/wallet/wallet.cpp(1 hunks)src/wallet/wallet.h(1 hunks)test/functional/feature_dbcrash.py(2 hunks)test/functional/wallet_basic.py(1 hunks)test/functional/wallet_listreceivedby.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/interfaces/wallet.h
- test/functional/wallet_basic.py
- test/functional/wallet_listreceivedby.py
- test/functional/feature_dbcrash.py
🧰 Additional context used
🪛 Cppcheck (2.10-2)
src/wallet/interfaces.cpp
[error] 225-225: Syntax Error
(internalAstError)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
🔇 Additional comments (19)
src/wallet/interfaces.cpp (3)
208-209: Improved error handling with early return patternThe new implementation uses the
FindAddressBookEntrymethod and an early return pattern for better error handling when an address is not found. This is a more straightforward approach than the previous iterator-based solution.
211-217: More direct access to entry propertiesThe code now directly accesses properties from the address book entry object rather than potentially using iterator dereferencing, making the code more readable and less error-prone.
221-228: Cleaner implementation with higher-order functionThe
getAddressesmethod has been updated to use theForEachAddrBookEntryhigher-order function, which:
- Makes the code more maintainable by abstracting the iteration logic
- Properly handles filtering of change addresses
- Uses a cleaner callback approach for populating the result vector
The addition of the
constqualifier to the method signature correctly reflects that this method doesn't modify the wallet state, which improves API clarity and enables compiler optimizations.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 225-225: Syntax Error
(internalAstError)
src/wallet/wallet.cpp (3)
2903-2909: Well-designed iterator with a clear lambda interfaceThis new method provides a clean abstraction for iterating through address book entries, passing each entry's components to a provided function. Using a function argument allows for flexible processing of address book entries without duplicating iteration logic.
2911-2925: Good implementation of address filtering with optional filter parameterThe
ListAddrBookAddressesmethod efficiently leverages the iterator pattern established byForEachAddrBookEntry. The implementation correctly handles the optional filter parameter with a sensible default, and properly enforces thread safety through theAssertLockHeld. The filtering logic is clear and concise.
2927-2939: Clean implementation for collecting unique labelsThis method properly uses a set for deduplication of labels, and correctly applies filtering based on the provided purpose parameter. The implementation avoids including change addresses in the results, which is appropriate for most UI and RPC use cases.
src/wallet/wallet.h (4)
796-802: Well-designed filtering structure addedGood introduction of the
AddrBookFilterstruct which provides a clean way to filter address book entries by label and to control whether change addresses are included. Usingstd::optionalfor the label filter is appropriate since filtering by label is optional.
804-807: Approve new address book listing methodThe
ListAddrBookAddressesmethod provides a flexible way to retrieve filtered address book entries, replacing the removedGetLabelAddressesmethod. Taking an optional filter parameter is a good design choice.
809-812: Approve labels listing methodThe method correctly retrieves all known labels for a specific purpose from the address book. This is a useful addition for wallet interfaces that need to display label lists.
814-819: Update documentation for ForEachAddrBookEntry to correctly reflect its behaviorThere is an inconsistency: the documentation claims that iteration stops when the callback returns false, yet the callback type is defined as
voidand the implementation iterates over all entries without checking any return value. Please update the comment to remove any reference to stopping on a false return value.src/wallet/rpcwallet.cpp (9)
954-954: Looks good.Introducing
std::optional<CTxDestination>clarifies intent and is a safe way to represent optional addresses. No concerns here.
1001-1001: Early return for change addresses.This logic is straightforward: skipping further processing for addresses flagged as change. No issues found.
1004-1004: Guard clause is valid.This return statement logically avoids processing non-tallied addresses when
!fIncludeEmpty.
1013-1013: Conditional check is straightforward.No functional concerns; the code is clear.
1032-1033: Loop usage is concise.Appending each txid’s hex to the list is correct. Looks good.
1042-1043: Optional usage is clean.Using
if (filtered_address)followed by dereferencing is safe and clear.
1045-1045: Lambda invocation approach is valid.No issues found in calling the provided function pointer.
1047-1048: Fallback logic is clear.Invoking
ForEachAddrBookEntrywhen no specific filter is set is appropriately placed.
1050-1051: Label-based tally loop is straightforward.The logic for aggregating amounts by label is coherent and correct.
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
utACK bb26584fb836ed10239e56644fbbd7ecd428224c
|
Hello @PastaPastaPasta , requesting review /approve |
|
Hello @PastaPastaPasta requesting review |
|
This pull request has conflicts, please rebase. |
|
This pull request has conflicts, please rebase. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/wallet/wallet.h (1)
814-819:⚠️ Potential issueDocumentation inconsistency in ForEachAddrBookEntry
The documentation comment states that iteration "Stops when the provided 'ListAddrBookFunc' returns false", but the function type is defined with a
voidreturn type, which can't return a boolean value to control iteration.Update the documentation to match the actual behavior, which appears to be iterating through all address book entries without an early termination condition.
- * Stops when the provided 'ListAddrBookFunc' returns false. + * Iterates through all address book entries and calls the provided function for each entry.
🧹 Nitpick comments (1)
src/wallet/rpcwallet.cpp (1)
579-580: Potential performance concern with membership check
This line usesstd::findon the vector, which has O(n) complexity. For large address sets, consider using a set for faster lookups.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between bb26584fb836ed10239e56644fbbd7ecd428224c and c5a70462a8af12a32593b5e5b1227e793f518391.
📒 Files selected for processing (10)
src/interfaces/wallet.h(1 hunks)src/wallet/bdb.cpp(0 hunks)src/wallet/interfaces.cpp(1 hunks)src/wallet/rpcwallet.cpp(9 hunks)src/wallet/wallet.cpp(1 hunks)src/wallet/wallet.h(1 hunks)src/wallet/walletdb.cpp(2 hunks)test/functional/feature_dbcrash.py(2 hunks)test/functional/wallet_basic.py(1 hunks)test/functional/wallet_listreceivedby.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/wallet/bdb.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- test/functional/wallet_listreceivedby.py
- src/interfaces/wallet.h
- test/functional/wallet_basic.py
- src/wallet/walletdb.cpp
- test/functional/feature_dbcrash.py
🧰 Additional context used
🧬 Code Definitions (4)
src/wallet/wallet.h (2)
src/wallet/interfaces.cpp (10)
dest(181-185)dest(181-181)dest(194-197)dest(194-194)dest(198-201)dest(198-198)dest(202-220)dest(202-205)dest(235-239)dest(235-235)src/interfaces/wallet.h (5)
dest(119-119)dest(125-125)dest(128-128)dest(131-134)dest(143-143)
src/wallet/wallet.cpp (1)
src/wallet/wallet.h (7)
func(819-819)filter(807-807)label(237-240)label(237-237)label(757-757)label(827-827)purpose(812-812)
src/wallet/rpcwallet.cpp (4)
src/wallet/wallet.h (10)
label(237-240)label(237-237)label(757-757)label(827-827)wallet(414-414)dest(610-610)address(857-857)address(859-859)filter(807-807)func(819-819)src/wallet/rpc/util.cpp (2)
LabelFromValue(116-122)LabelFromValue(116-116)src/script/standard.cpp (2)
ExtractDestination(169-197)ExtractDestination(169-169)src/core_write.cpp (2)
ValueFromAmount(31-42)ValueFromAmount(31-31)
src/wallet/interfaces.cpp (3)
src/wallet/rpcwallet.cpp (7)
entry(307-307)entry(1195-1195)entry(1229-1229)entry(1613-1613)entry(3184-3184)dest(3707-3707)dest(3707-3707)src/interfaces/wallet.h (8)
dest(119-119)dest(125-125)dest(128-128)dest(131-134)dest(143-143)name(346-346)name(349-349)label(109-109)src/wallet/wallet.h (9)
dest(610-610)dest(828-828)dest(830-830)dest(861-861)purpose(812-812)label(237-240)label(237-237)label(757-757)label(827-827)
🪛 GitHub Actions: Check Merge Fast-Forward Only
src/wallet/rpcwallet.cpp
[error] 1-1: Merge conflict in src/wallet/rpcwallet.cpp. Automatic merge failed; fix conflicts and then commit the result.
🪛 Cppcheck (2.10-2)
src/wallet/interfaces.cpp
[error] 225-225: Syntax Error
(internalAstError)
🔇 Additional comments (26)
src/wallet/interfaces.cpp (4)
208-209: Improved error handling using FindAddressBookEntryThe code now uses
FindAddressBookEntrywithallow_change=falseto check if an address exists in the address book, providing clearer error handling with an early return if the address is not found.
211-212: Better encapsulation with address book entry objectUsing
entry->GetLabel()instead of directly accessing label data through an iterator improves encapsulation and ensures consistent access patterns.
217-218: Consistent access pattern for address book dataRetrieving the purpose field via
entry->purposemaintains consistency with the updated approach to address book data access.
221-229: Enhanced method signature and implementation using ForEachAddrBookEntryThe
getAddressesmethod has been improved in two ways:
- The method is now marked
const, correctly indicating it doesn't modify the wallet state- The implementation now uses the new
ForEachAddrBookEntrymethod, which simplifies the code by replacing manual iteration with a callback-based approachThis refactoring makes the code more maintainable and follows better API design practices.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 225-225: Syntax Error
(internalAstError)
src/wallet/wallet.h (3)
797-802: Well-designed filter struct for address book operationsThe new
AddrBookFilterstruct provides a flexible way to filter address book entries with:
- Optional label filtering via
m_op_label- Control over including change addresses via
ignore_changeThis is a good design choice that allows for extensible filtering capabilities while maintaining backward compatibility.
804-807: New flexible method for retrieving address book destinationsThe
ListAddrBookAddressesmethod provides a clean API for retrieving filtered address book entries, replacing the previousGetLabelAddressesmethod with a more versatile solution that allows filtering by multiple criteria.
809-812: New utility method for listing address book labels by purposeThe
ListAddrBookLabelsmethod exposes a useful capability to retrieve all labels for a given purpose, making it easier for callers to work with address book data in a structured way.src/wallet/wallet.cpp (3)
2903-2909: Clean implementation of a generic address book iterator.This method provides a simple but powerful abstraction for iterating over address book entries, supporting the application of custom operations through the callback function parameter. This is a good example of the Iterator pattern that promotes code reuse and modularity.
2911-2925: Well-designed address filtering implementation.This method effectively leverages the
ForEachAddrBookEntryfunction to filter addresses based on optional criteria. The use of a lambda function to apply filtering logic is clean and the implementation correctly handles both the case when a filter is provided and when it's not.
2927-2939: Good implementation for retrieving unique labels.This method efficiently collects unique labels from the address book for a specific purpose. Using a set data structure ensures uniqueness of returned labels and skipping change addresses is the correct behavior for this use case.
src/wallet/rpcwallet.cpp (16)
543-543: Use of a local vector to store addresses
This change is straightforward and does not raise any concerns.
548-548: Switched from old address retrieval method toListAddrBookAddresses
This looks correct and consistent with the new address book approach.
559-559: Emplacing the single address
No issues found, the logic is straightforward.
954-954: Introduction of an optionalfiltered_address
No issues. This is a clear approach to conditionally filter results.
972-972: Early continue if depth is insufficient
No issues found.
974-974: Loop through transaction outputs
Implementation is correct.
979-979: Conditional check forfiltered_address
The logic is aligned with the new optional address filter usage.
984-984: Checkisminefilter
No issues, consistent usage with existing filter logic.
1000-1001: Lambda capturing address, label, and purpose
Filtering out change addresses is logical and correct.
1004-1005: Skipping if address is not tallied andfIncludeEmptyis false
No issues.
1007-1008: Checkisminefilter
Consistent with the approach for filtering addresses.
1014-1014: ExtractingnAmountfrom mapTally
Logic is correct.
1024-1026: Else block for non-label grouping
Implementation is clear, no issues.
1032-1033: Populate transaction IDs
Logic for enumerating transaction IDs is correct.
1040-1047: Handlingfiltered_addressvs. full address book
This usage ofForEachAddrBookEntryis consistent with the new approach. No concerns.
1050-1051: Handling label-based tally
Implementation to handle label grouping is correct.
knst
left a comment
knst
left a comment
There was a problem hiding this comment.
utACK c5a70462a8af12a32593b5e5b1227e793f518391
src/wallet/wallet.cpp
Outdated
There was a problem hiding this comment.
Note for myself: missing AssertLockHeld(cs_wallet); due to non-backported bitcoin#19833
…letBatch' is created c318211 walletdb: fix last client version update (furszy) bda8ebe wallet: don't read db every time that a new WalletBatch is created (furszy) Pull request description: Found it while was working on bitcoin#25297. We are performing a db read operation every time that a new `WalletBatch` is created, inside the constructor, just to check if the client version field is inside the db or not. As the client version field does not change in the entire db lifecycle, this operation can be done only once: The first time that the db is accessed/opened and the client version value can be cached. ACKs for top commit: achow101: ACK c318211 w0xlt: reACK bitcoin@c318211 Tree-SHA512: 7fb780c656e169e8eb21e7212242494a647f6506d6da2cca828703713d440d29c82bec9e7d2c410f37b49361226ccd80846d3eeb8168383d0c2a11d85d73bee2
…ng dust (instead of `acceptnonstdtxn=1`) 1770be7 test: pass `dustrelayfee=0` option for tests using dust (instead of `acceptnonstdtxn=1`) (Sebastian Falbesoner) Pull request description: By specifying the `dustrelayfee=0` option instead of the more generic `acceptnonstdtxn=1`, we can be more specific about what part of the transaction is non-standard and can be sure that all other aspects follow the standard policy. Note that for the test `feature_dbcrash.py`, the UTXO creation at the start of the test has to be split up to several txs to not exceed the tx standard size limit of 100k vbytes https://github.com/bitcoin/bitcoin/blob/4129c1375430dbfe8dd414868c43fceb3d091fc3/src/policy/policy.h#L26-L27 ACKs for top commit: MarcoFalke: review ACK 1770be7 Tree-SHA512: 5cb852a92883a7443ab7dc15b48efa76b5d1424b6b0da1fa6b075fbe9a83522e3ff60382d36c08d4b07143ed898c115614582474e37837332caaee73b0db0e47
d69045e test: add coverage for 'listreceivedbyaddress' no change addrs return (furszy) 324f00a refactor: 'ListReceived' use optional for filtered address (furszy) b459fc1 refactor: RPC 'ListReceived', encapsulate m_address_book access (furszy) fa9f2ab refactor: RPC 'listlabels', encapsulate 'CWallet::ListAddrBookLabels' functionality (furszy) 83e42c4 refactor: use 'ForEachAddrBookEntry' in RPC 'getaddressesbylabel' (furszy) 2b48642 refactor: use ForEachAddrBookEntry in interfaces::getAddresses (furszy) 032842a wallet: implement ForEachAddrBookEntry method (furszy) 09649bc refactor: implement general 'ListAddrBookAddresses' for addressbook destinations lookup (furszy) 192eb1e refactor: getAddress don't access m_address_book, use FindAddressEntry function (furszy) Pull request description: ### Context The wallet's `m_address_book` field is being accessed directly from several places across the sources. ### Problem Code structure wise, we shouldn't be accessing it directly. It could end up being modified by mistake (from a place that has nothing to do with the wallet like an RPC command or the GUI) and cause a bigger issue: like an address book entry 'purpose' string change, which if done badly (from 'send' to 'receive'), could end up in a user sharing a "receive" address that he/she doesn't own. ### Solution Encapsulate `m_address_book` access inside the wallet. ------------------------------------------------------- Extra Note: This is the initial step towards decoupling the address book functionality from the wallet's sources. In other words, the creation of the `AddressBookManager` (which will be coming in a follow-up PR). ACKs for top commit: achow101: ACK d69045e theStack: ACK d69045e ✅ w0xlt: ACK bitcoin@d69045e Tree-SHA512: dba17acd86f171b4e9af0223bbbcad380048570f6a2f6a92732a51f01abe8806debaf65c9e9e5569fa76a541903cbb50adcb5f56ef77858151c698ae6b218e2a
|
Hello @PastaPastaPasta , previously approved by @UdjinM6 @knst, had to rebase due to conflict , |
5 participants
bitcoin backports